Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Apply default pattern generation from first image instead of last #398

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

alecgeatches
Copy link
Contributor

With our current default pattern generation, we overwrite $bindings['image']['url'] and $bindings['image']['alt'] each time we encounter a new one. I'm working with a data source that returns several images, with some of them being optional. The configuration output mapping looks like this:

'type' => [
	'id' => [
		'name' => 'Product ID',
		'path' => '$.id',
		'type' => 'id',
	],
	'name' => [
		'name' => 'Name',
		'path' => '$.name',
		'type' => 'string',
	],
	'sku' => [
		'name' => 'SKU',
		'path' => '$.sku',
		'type' => 'string',
	],
	'description' => [
		'name' => 'Description',
		'path' => '$.fields.Description',
		'type' => 'string',
	],
	'image_url' => [
		'name' => 'Image URL',
		'path' => '$.defaultImage.url',
		'type' => 'image_url',
	],
	'image_alt_text' => [
		'name' => 'Image Alt Text',
		'path' => '$.defaultImage.alternateText',
		'type' => 'image_alt',
	],
	'Variant1Image__c' => [
		'name' => 'Variant1Image',
		'path' => '$.fields.Variant1Image__c',
		'type' => 'image_url',
	],
	'Variant2Image__c' => [
		'name' => 'Variant2Image',
		'path' => '$.fields.Variant2Image__c',
		'type' => 'image_url',
	],
	'Variant3Image__c' => [
		'name' => 'Variant3Image',
		'path' => '$.fields.Variant3Image__c',
		'type' => 'image_url',
	],
	/* ... */
];

I have the primary image URL and alt text at the top of the configuration, and some optional variant possibilities below. Prior to this PR, the auto-generated image that's selected by the default pattern would be Variant3Image__c (the least likely to have a value in my case) and image_url is never used.

This PR changes pattern generation to keep data about all images and instead construct the default pattern from the first encountered image and alt text. Note that this may cause the primary image's alt text to match an unrelated alt text from another part of the config, because bindings are evaluated individually. However, this is the same behavior as the current pattern generation so this is not a regression.

Copy link

Test this PR in WordPress Playground.

Copy link
Member

@chriszarate chriszarate left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok. This existing code is pretty suboptimal w/r/t images already. There's not guarantee that the alt text and image url correspond since we're just operating on the returned order. We're outgrowing this and probably need to give more agency to the user via the UI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants